Let `BackgroundProcessor` drive HTLC forwarding #3891

tnull · 2025-06-25T15:12:49Z

Closes #3768.
Closes #1101.

Previously, we'd require the user to manually call process_pending_htlc_forwards as part of PendingHTLCsForwardable event handling. Here, we rather move this responsibility to BackgroundProcessor, which simplifies the flow and allows us to implement reasonable forwarding delays on our side rather than delegating to users' implementations.

Note this also introduces batching rounds rather than calling process_pending_htlc_forwards individually for each PendingHTLCsForwardable event, which had been unintuitive anyways, as subsequent PendingHTLCsForwardable could lead to overlapping batch intervals, resulting in the shortest timespan 'winning' every time, as process_pending_htlc_forwards would of course handle all pending HTLCs at once.

To this end, we implement random sampling of batch delays from a log-normal distribution with a mean of 50ms and drop the PendingHTLCsForwardable event.

~~Draft for now as I'm still cleaning up the code base as part of the final commit dropping PendingHTLCsForwardable.~~

ldk-reviews-bot · 2025-06-25T15:12:52Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

joostjager · 2025-06-25T16:46:14Z

Does this in any way limit users to not have delays or not have batching? Assuming that's what they want.

tnull · 2025-06-25T16:58:39Z

Does this in any way limit users to not have delays or not have batching? Assuming that's what they want.

On the contrary actually: it effectively reduces the (mean and min forwarding) delay quite a bit, which we can allow as we're gonna add larger receiver-side delays in the next step. And, while it get's rid of the event, users are still free to call process_pending_htlc_forwards on a faster schedule if they really want to. IMO, this should result in a win-win situation: substantially reduced forwarding delays on average and by default, while still considerably improving receiver anonymity.

joostjager · 2025-06-26T09:11:32Z

Isn't it the case that without the event, as a user you are forced to "poll" for forwards, making extra delays unavoidable?

tnull · 2025-06-26T09:17:20Z

Isn't it the case that without the event, as a user you are forced to "poll" for forwards, making extra delays unavoidable?

LDK always processes HTLCs in batches (note that process_pending_htlcs never allowed to just forward a single HTLC, for good reason). Having some batching delay makes a lot of sense in any scenario. And given that 'polling' is really cheap, users could consider doing that frequently. But, they really shouldn't try to skip the batching entirely as IO overhead/delay would come to bite them (especially on more busy forwarding nodes), and of course since they should be 'good citizens' providing some privacy by default for the network.

joostjager · 2025-06-26T09:26:58Z

Polling may be cheap, but forcing users to poll when there is an event mechanism available, is that really the right choice? Perhaps the event is beneficial for testing, debugging and monitoring too?

tnull · 2025-06-26T09:32:32Z

Polling may be cheap, but forcing users to poll when there is an event mechanism available, is that really the right choice? Perhaps the event is beneficial for testing, debugging and monitoring too?

The event never featured any information so is not helpful for debugging or 'informational' purposes. Plus, it means at least 1-2 more rounds of ChannelManager persistence, just to queue and remove the event. So since we don't need it anymore, we should def. drop it in production. As you know I was on the fence whether to drop it for testing, but now went this way, especially given that nobody indicated a strong opinion either way. If we indeed want to introspect the holding cell during testing (or, e.g., in fuzzing), we should add another approach to do it, but that's up for discussion.

joostjager · 2025-06-26T09:41:28Z

But at least the event could wake up the background processor, where as now nothing is waking it up for forwards and the user is forced to call into channel manager at a high frequency? Not sure if there is a lighter way to wake up the bp without persistence involved.

Also if you have to call into channel manager always anyway, aren't there more events/notifiers that can be dropped?

As you know I was on the fence whether to drop it for testing, but now went this way, especially given that nobody indicated a strong opinion either way.

I may have missed this deciding moment.

If the assertions were useless to begin with, no problem dropping them of course. I can imagine though that at some points, a peek into the pending htlc state is still required to not reduce the coverage of the tests?

tnull · 2025-06-26T09:46:58Z

But at least the event could wake up the background processor, where as now nothing is waking it up for forwards and the user is forced to call into channel manager at a high frequency? Not sure if there is a lighter way to wake up the bp without persistence involved.

Also if you have to call into channel manager always anyway, aren't there more events/notifiers that can be dropped?

As you know I was on the fence whether to drop it for testing, but now went this way, especially given that nobody indicated a strong opinion either way.

I may have missed this deciding moment.

Again, the default behavior we had intended to switch to for quite some time is to introduce batching intervals (especially given that the current event-based approach was essentially broken/race-y). This is what is implemented here. If users want to bend the recommended/default approach they are free to do so, but I don't think it makes sense to keep all the legacy codepaths, including persistence overhead, around if it's not used anymore.

If the assertions were useless to begin with, no problem dropping them of course. I can imagine though that at some points, a peek into the pending htlc state is still required to not reduce the coverage of the tests?

I don't think this is generally the case, no. The 'assertion' that is mainly dropped is 'we generated an event', every thing else remains the same.

joostjager · 2025-06-26T10:25:37Z

Again, the default behavior we had intended to switch to for quite some time is to introduce batching intervals (especially given that the current event-based approach was essentially broken/race-y). This is what is implemented here. If users want to bend the recommended/default approach they are free to do so, but I don't think it makes sense to keep all the legacy codepaths, including persistence overhead, around if it's not used anymore.

This doesn't rule out a notification when there's something to forward, to at least not keep spinning when there's nothing to do?

tnull · 2025-06-27T09:31:17Z

Finished for now with the test refactoring post-dropping PendingHTLCsForwardable event. This should be good for a first round of (concept) review. Whether or not we should add a notifier on top is up for debate.

ldk-reviews-bot · 2025-06-27T09:40:24Z

✅ Added second reviewer: @valentinewallace

joostjager · 2025-06-27T13:22:18Z

lightning-background-processor/src/lib.rs

@@ -360,12 +376,24 @@ macro_rules! define_run_body {
 				break;
 			}

+			if $timer_elapsed(&mut last_forwards_processing_call, cur_batch_delay) {
+				$channel_manager.get_cm().process_pending_htlc_forwards();


Looked a bit closer at this function. There is a lot of logic in there. Also various locks obtained.

ldk-reviews-bot · 2025-06-30T00:00:43Z

🔔 1st Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-07-02T00:01:33Z

🔔 2nd Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Now that we have `BackgroundProcessor` drive the batch forwarding of HTLCs, we implement random sampling of batch delays from a log-normal distribution with a mean of 50ms.

tnull · 2025-07-14T11:34:23Z

Now added a commit that adds a simple is_pending_htlc_processing helper that we use to skip waking on the batch delay and calling into process_pending_htlcs_forwards.

tnull · 2025-07-14T11:47:06Z

lightning/src/ln/channelmanager.rs

+	pub fn is_pending_htlc_processing(&self) -> bool {
+		let has_forward_htlcs = !self.forward_htlcs.lock().unwrap().is_empty();
+		let has_decode_update_add_htlcs = !self.decode_update_add_htlcs.lock().unwrap().is_empty();
+		let has_outbound_needing_abandon = self.pending_outbound_payments.needs_abandon();


This seems pre-existing, but aren't we also pending processing if we have auto-retryable outbound payments, not only if they need abandoning (i.e., what we then process via check_retry_payments)? Not sure if I'm missing something here? (cc @valentinewallace)

I believe on main we push a forwardable event when an HTLC fails, thus triggering the check_retry_payments call when the event is handled. We might need to add a method to outbound_payments to see whether any payments are is_auto_retryable_now().

.. as `forward_htlcs` now does the same thing

.. as `fail_htlcs_backwards_internal` now does the same thing

We move the code into the `optionally_notify` closure, but maintain the behavior for now. In the next step, we'll use this to make sure we only repersist when necessary. Best reviewed via `git diff --ignore-all-space`

We skip repersisting `ChannelManager` when nothing is actually processed.

We add a reenatrancy guard to disallow entering `process_pending_htlc_forwards` multiple times. This makes sure that we'd skip any additional processing calls if a prior round/batch of processing is still underway.

valentinewallace · 2025-07-14T15:57:00Z

Can we squash? I'm also tempted to ask you to break up this PR 😅 seems the complexity has grown since it's at +3000 now

valentinewallace · 2025-07-14T17:03:01Z

lightning-background-processor/src/fwd_batch.rs

+	const FALLBACK_DELAY: u16 = 50;
+	let delay;
+
+	#[cfg(feature = "std")]
+	{
+		const USIZE_LEN: usize = core::mem::size_of::<usize>();
+		let mut random_bytes = [0u8; USIZE_LEN];
+		possiblyrandom::getpossiblyrandom(&mut random_bytes);
+
+		let index = usize::from_be_bytes(random_bytes) % FWD_DELAYS_MILLIS.len();
+		delay = *FWD_DELAYS_MILLIS.get(index).unwrap_or(&FALLBACK_DELAY)
+	}
+	#[cfg(not(feature = "std"))]
+	{
+		delay = FALLBACK_DELAY
+	}


Can you document why we have different behavior in no-std?

valentinewallace · 2025-07-14T17:13:59Z

lightning/src/ln/channelmanager.rs

@@ -6358,13 +6357,37 @@ where
 		}
 	}

+	/// Returns whether we have pending HTLC forwards that need to be processed via
+	/// [`Self::process_pending_htlc_forwards`].
+	pub fn is_pending_htlc_processing(&self) -> bool {


s/is_pending_htlc_processing/needs_pending_htlc_processing? Right now to me it's ambiguous whether it means we're currently processing htlcs.

valentinewallace · 2025-07-14T17:18:58Z

lightning/src/ln/functional_test_utils.rs

-/// Handles a PendingHTLCsForwardable and HTLCHandlingFailed event
-macro_rules! expect_pending_htlcs_forwardable_and_htlc_handling_failed {
+/// Processes any HTLC forwards and handles an expected [`Event::HTLCHandlingFailed`].
+macro_rules! process_htlcs_and_expect_htlc_handling_failed {


Now that we have the method that checks if forwarding is needed, can we revert some of the diff in tests and change expect_pending_htlcs_forwardable to call ChannelManager::is_pending_htlc_processing? Seems like strictly better test coverage and easier review. Matt got me overthinking the test coverage changes...

valentinewallace · 2025-07-14T17:27:01Z

lightning/src/ln/channelmanager.rs

+											$htlc.prev_hop.counterparty_node_id;
+										let channel_id = prev_channel_id;
+										let outpoint = prev_funding_outpoint;
+										let htlc_id = $htlc.prev_hop.htlc_id;


FWIW I requested this and also prefer only pulling out some vars

tnull marked this pull request as draft June 25, 2025 15:12

tnull force-pushed the 2025-06-batch-forwarding-delays branch from ceb3335 to 9ba691c Compare June 26, 2025 08:13

tnull force-pushed the 2025-06-batch-forwarding-delays branch from 9ba691c to b38c19e Compare June 26, 2025 09:49

tnull force-pushed the 2025-06-batch-forwarding-delays branch from c1a0b35 to d35c944 Compare June 26, 2025 13:17

tnull added this to Weekly Goals Jun 26, 2025

tnull self-assigned this Jun 26, 2025

tnull force-pushed the 2025-06-batch-forwarding-delays branch from d35c944 to c21aeab Compare June 27, 2025 09:29

tnull requested a review from TheBlueMatt June 27, 2025 09:29

tnull marked this pull request as ready for review June 27, 2025 09:29

tnull removed the request for review from TheBlueMatt June 27, 2025 09:36

tnull moved this to Goal: Merge in Weekly Goals Jun 27, 2025

ldk-reviews-bot requested a review from valentinewallace June 27, 2025 09:40

tnull requested review from TheBlueMatt and removed request for TheBlueMatt June 27, 2025 09:51

joostjager reviewed Jun 27, 2025

View reviewed changes

tnull force-pushed the 2025-06-batch-forwarding-delays branch from c21aeab to e2ad6ca Compare July 2, 2025 09:55

tnull added 15 commits July 14, 2025 13:31

Randomly draw forwarding delays

128f118

Now that we have `BackgroundProcessor` drive the batch forwarding of HTLCs, we implement random sampling of batch delays from a log-normal distribution with a mean of 50ms.

f Fix clippy lint

34434d0

f Account for usize not always being 8 bytes

827e264

f Implement fallback delay on no-std

78c596b

Drop PendingHTLCsForwardable event

2f6dcd9

f Fix length check in expect_payment_failed_condition_event

a208a18

f Drop expect_pending_htlcs_forwardable

9aa646f

f Drop expect_pending_htlcs_forwardable_conditions macro

0a79650

f Drop expect_pending_htlcs_forwardable_.._ignore macro

0bd1d34

f Drop expect_pending_htlcs_forwardable_ignore macro

5769fd3

f Rename expect..and_htlc_handling_failed macro

6080a87

f Rename expect_pending_htlcs_forwardable_conditions

2a7aaa9

f Adjust helper docs

c8bb8ab

f Try fix fuzzer

b6f6716

f Keep OutboundPayments::needs_abandon

2123d73

tnull force-pushed the 2025-06-batch-forwarding-delays branch from 64cb370 to d7387bf Compare July 14, 2025 11:34

tnull commented Jul 14, 2025

View reviewed changes

tnull force-pushed the 2025-06-batch-forwarding-delays branch 3 times, most recently from 4519c80 to e4142d6 Compare July 14, 2025 14:00

tnull added 6 commits July 14, 2025 16:26

f Fix threaded_payment_retries

eff42b1

Drop unnecessary forward_htlcs_without_forward_event

6a70150

.. as `forward_htlcs` now does the same thing

Drop unnecessary fail_htlcs_.._without_forwarding_event

03f3422

.. as `fail_htlcs_backwards_internal` now does the same thing

Use optionally_notify in process_pending_htlc_forwards

3fada24

We move the code into the `optionally_notify` closure, but maintain the behavior for now. In the next step, we'll use this to make sure we only repersist when necessary. Best reviewed via `git diff --ignore-all-space`

Skip unnecessary persists in process_pending_htlc_forwards

cf90ad5

We skip repersisting `ChannelManager` when nothing is actually processed.

Add reenatrancy guard for process_pending_htlc_forwards

dac90af

We add a reenatrancy guard to disallow entering `process_pending_htlc_forwards` multiple times. This makes sure that we'd skip any additional processing calls if a prior round/batch of processing is still underway.

tnull force-pushed the 2025-06-batch-forwarding-delays branch from e4142d6 to dac90af Compare July 14, 2025 14:26

valentinewallace reviewed Jul 14, 2025

View reviewed changes

Let BackgroundProcessor drive HTLC forwarding #3891

Are you sure you want to change the base?

Let BackgroundProcessor drive HTLC forwarding #3891

Conversation

tnull commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joostjager commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joostjager commented Jun 26, 2025

Uh oh!

tnull commented Jun 26, 2025

Uh oh!

joostjager commented Jun 26, 2025

Uh oh!

tnull commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joostjager commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joostjager commented Jun 26, 2025

Uh oh!

tnull commented Jun 27, 2025

Uh oh!

ldk-reviews-bot commented Jun 27, 2025

Uh oh!

joostjager Jun 27, 2025

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Jun 30, 2025

Uh oh!

ldk-reviews-bot commented Jul 2, 2025

Uh oh!

tnull commented Jul 14, 2025

Uh oh!

tnull Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace commented Jul 14, 2025

Uh oh!

valentinewallace Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

valentinewallace Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

valentinewallace Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Let `BackgroundProcessor` drive HTLC forwarding #3891

Let `BackgroundProcessor` drive HTLC forwarding #3891

tnull commented Jun 25, 2025 •

edited

Loading

ldk-reviews-bot commented Jun 25, 2025 •

edited

Loading

joostjager commented Jun 25, 2025 •

edited

Loading

tnull commented Jun 25, 2025 •

edited

Loading

tnull commented Jun 26, 2025 •

edited

Loading

joostjager commented Jun 26, 2025 •

edited

Loading

tnull commented Jun 26, 2025 •

edited

Loading

tnull Jul 14, 2025 •

edited

Loading

valentinewallace Jul 14, 2025 •

edited

Loading

valentinewallace Jul 14, 2025 •

edited

Loading